Skip to content

feat: implement pv primitive#27

Merged
sourcehawk merged 26 commits intomainfrom
feature/pv-primitive
Mar 25, 2026
Merged

feat: implement pv primitive#27
sourcehawk merged 26 commits intomainfrom
feature/pv-primitive

Conversation

@sourcehawk
Copy link
Owner

Implements the pv Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds pv primitive package under pkg/primitives/pv/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Does not modify shared files

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new pv primitive to the framework to manage Kubernetes PersistentVolume resources using the existing primitive patterns (builder/resource/mutator/flavors/status handler), plus documentation and an example.

Changes:

  • Introduces pkg/primitives/pv/ with builder, resource wrapper, mutator, flavors, and operational status handler (+ unit tests).
  • Adds a shared PVSpecEditor under pkg/mutation/editors/ to support typed PV spec mutations (+ tests).
  • Adds a runnable examples/pv-primitive/ example and new documentation page docs/primitives/pv.md.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/primitives/pv/resource.go PV integration resource wrapper + default PV field applicator preserving immutable fields
pkg/primitives/pv/builder.go Fluent builder for PV resources with mutations, flavors, operational status handler, extractors
pkg/primitives/pv/mutator.go PV mutator implementing the “plan-and-apply” pattern with metadata/spec edit ordering
pkg/primitives/pv/handlers.go Default operational status mapping from PV phases to framework operational statuses
pkg/primitives/pv/flavors.go PV-specific wrappers for common field-application flavors
pkg/primitives/pv/*_test.go Unit tests for builder, mutator ordering, flavors, and status handler
pkg/mutation/editors/pvspec.go New shared typed editor for PersistentVolumeSpec
pkg/mutation/editors/pvspec_test.go Unit tests for the new PVSpecEditor
examples/pv-primitive/main.go Demonstration program showing PV mutation behavior and status handler output
examples/pv-primitive/resources/pv.go Example factory wiring PV builder + example feature mutations + flavors
examples/pv-primitive/features/mutations.go Example feature-gated PV mutations for the demo
examples/pv-primitive/app/controller.go Example package-level commentary about PV ownership constraints
examples/pv-primitive/README.md Example documentation and run instructions
docs/primitives/pv.md New primitive documentation page describing PV builder/mutation/status/flavors

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Added focused unit tests for DefaultFieldApplicator covering create-vs-update behavior, immutable field preservation (PersistentVolumeSource, VolumeMode, ClaimRef), nil immutable field handling, and verifying the desired object is not mutated (pkg/primitives/pv/resource_test.go)

Intentionally ignored:

  • PR description/checklist accuracy comment (pvspec.go:6): This is about PR description text, not code. The PVSpecEditor in pkg/mutation/editors is a deliberate addition following the same pattern as ObjectMetaEditor. The PR description should be updated by the author to reflect this shared package addition, but no code change is needed.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 22, 2026 19:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Line 283 (pv.md): Added cluster-scoped owner constraint documentation to the Guidance section, explaining that PVs require a cluster-scoped owner for controller references and listing workarounds for namespace-scoped components.
  • Line 15 (pv.md) / primitives index: Added the PV primitive to the Built-in Primitives table and PVSpecEditor to the Mutation Editors table in docs/primitives.md, making the PV docs discoverable from the central index.

Intentionally ignored:

  • pvspec.go (PR description checklist): This is a meta-comment about the PR description text, not a code issue. The PR description can be updated separately by the author.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 03:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • builder.go:140 — Refactored Builder.Build() to delegate to generic.IntegrationBuilder.Build(). Added MarkClusterScoped() and WithCustomOperationalStatus() calls in NewBuilder(), removed the custom validate() method. This ensures ValidateBase() runs the full shared validation (identity, applicator, mutator-factory checks) and keeps PV consistent with other primitives.
  • docs/primitives.md:135 (table alignment) — Fixed spacing in the PV row to align with other rows in the built-in primitives table.

Intentionally ignored:

  • docs/primitives.md:135 (PR description checklist) — This comment asks to update the PR description/checklist to reflect that shared files are modified. This is a PR metadata issue, not a code change. The PR author should update the description accordingly.

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 16:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 16:59
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Used Quantity.Equal instead of assert.Equal for resource.Quantity comparisons in resource_test.go (line 48 and also line 127 which had the same issue)
  • Updated docs/primitives/pv.md (line 291) to reflect actual framework behavior: ownerReferences are skipped (not API-rejected) for cluster-scoped resources with namespace-scoped owners
  • Updated examples/pv-primitive/app/controller.go (line 8) package comment to reflect conditional ownerReference behavior
  • Changed examples/pv-primitive/README.md (line 9) from "Data Extraction" to "Result Inspection" to match actual example code

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

sourcehawk and others added 8 commits March 23, 2026 20:16
Introduces a typed editor for corev1.PersistentVolumeSpec with methods
for capacity, access modes, reclaim policy, storage class, mount options,
volume mode, and node affinity.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the PV primitive as a cluster-scoped Integration resource:
- Builder validates Name only (no namespace for cluster-scoped resources)
- DefaultFieldApplicator preserves immutable fields on existing PVs
  (volume source, volume mode, claim ref)
- DefaultOperationalStatusHandler reports status based on PV phase
- Mutator with plan-and-apply pattern for metadata and spec edits
- Flavors for preserving external labels and annotations

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the PV primitive's integration lifecycle, cluster-scoped
identity, immutable field preservation, mutation pipeline, editors,
flavors, operational status mapping, and usage guidance.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates PV builder, mutations, flavors, and operational status:
- Cluster-scoped identity (no namespace)
- Immutable field preservation on existing PVs
- Boolean-gated mutations for reclaim policy and mount options
- PreserveCurrentAnnotations flavor for external fields
- Operational status mapping from PV phases

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix compile error: use Spec.PersistentVolumeSource.HostPath instead of
  non-existent Spec.HostPath field, with nil check
- Update README to accurately describe what the example does (in-memory
  mutations, not fake client; operational status, not status conditions)
- Fix controller.go comment to remove reference to non-existent
  SkipOwnerRef option

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover create-vs-update behavior, immutable field preservation
(PersistentVolumeSource, VolumeMode, ClaimRef), nil immutable field
handling, and verify the desired object is not mutated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Comment on lines +53 to +58
func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) {
if edit == nil {
return
}
m.active.metadataEdits = append(m.active.metadataEdits, edit)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If a caller forgets to call BeginFeature() first (possible since NewMutator leaves active nil), this will panic at runtime. Consider making the mutator safe-by-default by auto-starting an initial plan (e.g., initialize plans/active in NewMutator, or lazily call BeginFeature() inside these methods when m.active == nil) so misuse can’t crash reconciliation.

Copilot uses AI. Check for mistakes.
Comment on lines +68 to +73
func (m *Mutator) EditPVSpec(edit func(*editors.PVSpecEditor) error) {
if edit == nil {
return
}
m.active.specEdits = append(m.active.specEdits, edit)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If a caller forgets to call BeginFeature() first (possible since NewMutator leaves active nil), this will panic at runtime. Consider making the mutator safe-by-default by auto-starting an initial plan (e.g., initialize plans/active in NewMutator, or lazily call BeginFeature() inside these methods when m.active == nil) so misuse can’t crash reconciliation.

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +15
// A PV in the Pending phase is reported as OperationPending. A PV in the
// Released or Failed phase is reported as OperationFailing.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment refers to OperationPending / OperationFailing, but the implementation returns concepts.OperationalStatusPending / concepts.OperationalStatusFailing. Updating the comment to match the actual status names will avoid confusion for readers searching for those identifiers.

Suggested change
// A PV in the Pending phase is reported as OperationPending. A PV in the
// Released or Failed phase is reported as OperationFailing.
// A PV in the Pending phase is reported as concepts.OperationalStatusPending. A PV in the
// Released or Failed phase is reported as concepts.OperationalStatusFailing.

Copilot uses AI. Check for mistakes.

| Capability | Detail |
| ------------------------- | -------------------------------------------------------------------------------------------------- |
| **Integration lifecycle** | Reports `Operational`, `OperationPending`, or `OperationFailing` based on the PV's phase |
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc uses OperationPending / OperationFailing, but the code (and tests) use concepts.OperationalStatusPending / concepts.OperationalStatusFailing. Please align the documentation terminology with the actual framework statuses (and keep it consistent with handlers.go and other primitive docs) to prevent users from looking for non-existent status values.

Suggested change
| **Integration lifecycle** | Reports `Operational`, `OperationPending`, or `OperationFailing` based on the PV's phase |
| **Integration lifecycle** | Reports `Operational`, `concepts.OperationalStatusPending`, or `concepts.OperationalStatusFailing` based on the PV's phase |

Copilot uses AI. Check for mistakes.
Update documentation and handler comments to use the actual
concepts.OperationalStatusPending / concepts.OperationalStatusFailing /
concepts.OperationalStatusOperational identifiers instead of the
shorthand OperationPending / OperationFailing / Operational names.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • handlers.go L14-15: Updated comment to use concepts.OperationalStatusPending / concepts.OperationalStatusFailing instead of OperationPending / OperationFailing
  • docs/primitives/pv.md L11: Updated capabilities table to use full concepts.OperationalStatus* identifiers
  • docs/primitives/pv.md L197-199: Updated operational status table to use OperationalStatusPending / OperationalStatusFailing / OperationalStatusOperational instead of shorthand names
  • docs/primitives/pv.md L243: Updated guidance section to use consistent status names

Intentionally ignored:

  • mutator.go L58/L73 (nil m.active panic): All other primitives (deployment, configmap) follow the same pattern — BeginFeature() is called by the framework mutation pipeline before any edits are registered. Adding nil-safety only to the PV mutator would be inconsistent with the codebase convention. The NewMutator godoc already documents that BeginFeature must be called first.

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings March 25, 2026 18:58
@sourcehawk sourcehawk merged commit e73a8a1 into main Mar 25, 2026
4 checks passed
@sourcehawk sourcehawk deleted the feature/pv-primitive branch March 25, 2026 18:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Comment on lines +44 to +47
func (m *Mutator) BeginFeature() {
m.plans = append(m.plans, featurePlan{})
m.active = &m.plans[len(m.plans)-1]
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If BeginFeature() hasn’t been called (or if future refactors call these before BeginFeature()), this will panic at runtime. Consider making the mutator resilient by ensuring an active plan exists (e.g., lazily calling BeginFeature() the first time an edit is registered), or by failing fast with a dedicated, clearer panic/message rather than a nil-pointer dereference.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +32
type Mutator struct {
pv *corev1.PersistentVolume

plans []featurePlan
active *featurePlan
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing active as a pointer to a slice element is fragile because any future append/re-slice patterns on m.plans could invalidate pointers and make bugs hard to debug. A more robust approach is to store an activeIndex int (or *featurePlan allocated independently), and reference m.plans[m.activeIndex] when appending edits.

Copilot uses AI. Check for mistakes.
Comment on lines +44 to +47
func (m *Mutator) BeginFeature() {
m.plans = append(m.plans, featurePlan{})
m.active = &m.plans[len(m.plans)-1]
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing active as a pointer to a slice element is fragile because any future append/re-slice patterns on m.plans could invalidate pointers and make bugs hard to debug. A more robust approach is to store an activeIndex int (or *featurePlan allocated independently), and reference m.plans[m.activeIndex] when appending edits.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants